Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: explicitly allocate backing stores for v8 stat buffers #30946

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

This fixes flaky tests that crashed because the allocations ended
up at positions of previously allocated ArrayBuffers that were
still in the backing store table. In particular, there was a race
condition window between destroying a Worker thread’s Environment
and destroying its Isolate in which the underlying memory was
already released but the ArrayBuffer was still existent, meaning
that new memory could be allocated at the address of the previous
ArrayBuffer.

Refs: #30782

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This fixes flaky tests that crashed because the allocations ended
up at positions of previously allocated `ArrayBuffer`s that were
still in the backing store table. In particular, there was a race
condition window between destroying a Worker thread’s `Environment`
and destroying its `Isolate` in which the underlying memory was
already released but the `ArrayBuffer` was still existent, meaning
that new memory could be allocated at the address of the previous
`ArrayBuffer`.

Refs: nodejs#30782
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 13, 2019
@addaleax
Copy link
Member Author

This addresses an issue that currently makes Worker tests flaky on various OSes in CI. Please feel free to 👍 this comment to approve fast-tracking.

@nodejs-github-bot
Copy link
Collaborator

@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 13, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2019
gabrielschulhof pushed a commit that referenced this pull request Dec 14, 2019
This fixes flaky tests that crashed because the allocations ended
up at positions of previously allocated `ArrayBuffer`s that were
still in the backing store table. In particular, there was a race
condition window between destroying a Worker thread’s `Environment`
and destroying its `Isolate` in which the underlying memory was
already released but the `ArrayBuffer` was still existent, meaning
that new memory could be allocated at the address of the previous
`ArrayBuffer`.

Refs: #30782
PR-URL: #30946
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gabrielschulhof
Copy link
Contributor

Landed in d06efaf.

@addaleax addaleax deleted the backing-store-clashes branch December 14, 2019 15:52
@addaleax
Copy link
Member Author

Marking as dont-land-on because this fixes an issue with a semver-major PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants